@RaymondLim just letting you know I'm going to make some tweaks to this PR now.

@RaymondLim visual tweaks done.

Thanks @larz0. I forgot to commit the svg file and you just submitted with your visual tweaking.

To me

@RaymondLim Just starting to look at this. A couple of issues I noticed:

• I had previously had an exclusion filter set up (for node_modules). When I launched your branch, the picker button showed that properly, but when I clicked on the dropdown, the list of filters was empty (the existing filter didn't show up). I chose "Don't Exclude Files", then chose "New Exclusion Set..." and added one for node_modules. After I did that, two node_modules entries showed up in the list.
• Since the name label is optional, I wonder if it would make sense to set focus to the main text area in the dialog when it comes up, instead of setting focus to the name label. (In fact, perhaps we should consider putting the name below, since it's less important than the actual list of folders, and since the description text above is about the main text area, not the name label.)
• It seems to me like the currently selected exclusion set should get a checkmark in the dropdown list.
• It's hard to figure out how to close the dropdown without also dismissing the Find in Files bar:
  • Clicking on the dropdown button when the list is already open doesn't close the list - it just reopens the list again. I think it should act like a toggle.
  • If I click outside the menu elsewhere in the document to close it, it closes the Find in Files bar. We should consider locking the Find in Files bar open in this case, and forcing focus back into it when the menu is closed - though that might be tricky to implement properly.

(/cc @larz0 to consider some of the UI issues mentioned in the comment above)

@njx I addressed most of your feedback with the following exceptions.

• Not yet create a template for list item.

• I had previously had an exclusion filter set up (for node_modules). When I launched your branch, the picker button showed that properly, but when I clicked on the dropdown, the list of filters was empty (the existing filter didn't show up). I chose "Don't Exclude Files", then chose "New Exclusion Set..." and added one for node_modules. After I did that, two node_modules entries showed up in the list.

Fixed.

• Since the name label is optional, I wonder if it would make sense to set focus to the main text area in the dialog when it comes up, instead of setting focus to the name label. (In fact, perhaps we should consider putting the name below, since it's less important than the actual list of folders, and since the description text above is about the main text area, not the name label.)

Fixed.

• It seems to me like the currently selected exclusion set should get a checkmark in the dropdown list.
  Implemented.
• It's hard to figure out how to close the dropdown without also dismissing the Find in Files bar:
  • Clicking on the dropdown button when the list is already open doesn't close the list - it just reopens the list again. I think it should act like a toggle.

Fixed, an existing bug that you can also reproduce in CSS New Rule button.

• If I click outside the menu elsewhere in the document to close it, it closes the Find in Files bar. We should consider locking the Find in Files bar open in this case, and forcing focus back into it when the menu is closed - though that might be tricky to implement properly.

Partially fixed by not closing Find bar, but was unable to restore focus to the Search text field.

The UI changes look good, but a couple of notes:

Partially fixed by not closing Find bar, but was unable to restore focus to the Search text field.

This is problematic, since we assume the Find bar is closed whenever focus is outside it. If you get into this case, then hitting Esc won't close the Find bar, for example. It's probably worth figuring out how to set focus back into the input field.

(Eventually we want to change this so the find bar can stay open when focus is in the document...we wouldn't have to fix anything for Find in Files specifically, I think, but we would have to make some changes for that to work for single-file Find/Replace, and we wouldn't want the two to behave inconsistently with each other.)

One other very minor note...the positioning of the "x" looks wrong on hover - it's slightly off-center relative to the checkmark. Maybe have @larz0 take a look.

I'll take a look at the code itself now.

Re-reviewed. My general comment would be to be careful about overly-general jQuery selectors - make sure you're looking specifically for the item you know you're looking for - and also to encapsulate UI knowledge where possible, rather than having clients know about the internal UI implementation of the component they're using.

@njx Ready for re-review.

Hmmm, I just realized a UI issue with the checkmark. Because it's a checkmark, you might think that selecting the same item again would toggle it off. (That's especially true if there's only one item. It might not be immediately obvious that the way you uncheck it is to select "Don't Exclude Files".)

I don't think that needs to prevent us merging this now, but we might want @larz0 to try it out and see how it feels. Should be a small bugfix if we want to implement that (and/or take the checkmark back out, though I think it's better to keep it in).

Changes look good. Just my one comment about the "refresh" event.

@njx I think it's fine because the control is a selector and not a menu item of a menu.

@njx. I find a way to move the triggerHandler call into _renderList as you suggested, so ready for the final review (I think).

@RaymondLim Changes look good. I just had the one comment about documenting the event - once you've added that comment, feel free to go ahead and merge.

(Oops, that comment was on the commit, not the PR)